- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.4k
 
refactor(emoji): centralize emoji rendering into app/components/Emoji… #6643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
refactor(emoji): centralize emoji rendering into app/components/Emoji… #6643
Conversation
          
WalkthroughAdds a reusable Emoji component at  Changes
 Sequence Diagram(s)sequenceDiagram
  autonumber
  actor UI as UI
  participant Picker as EmojiPickerEmoji
  participant Shared as app/components/Emoji/Emoji
  participant Shortname as useShortnameToUnicode
  participant Custom as CustomEmoji
  Note right of Picker: wrapper selects style then delegates
  UI->>Picker: render({ emoji, size, style })
  Picker->>Shared: render({ emoji, size, style })
  alt emoji is string
    Shared->>Shortname: formatShortnameToUnicode(trimmed or :shortname:)
    Shortname-->>Shared: unicodeChar
    Shared-->>UI: <Text style(fontSize=size)>{leading + unicodeChar + trailing}</Text>
  else emoji is ICustomEmoji
    Shared->>Custom: render({ emoji, width:size, height:size, style })
    Custom-->>Shared: <Image />
    Shared-->>UI: <CustomEmoji />
  end
    Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (5 passed)✅ Passed checks (5 passed)
 Poem
 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
 📒 Files selected for processing (3)
 🚧 Files skipped from review as they are similar to previous changes (3)
 ✨ Finishing touches
 🧪 Generate unit tests
 Comment   | 
    
| 
           Hi 👋, this PR is ready for review.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️  Outside diff range comments (1)
app/containers/markdown/components/emoji/Emoji.tsx (1)
55-56: Fix potential undefined access when sanitizing custom emoji name
block.value?.value.replace(...)can throw whenblock.valueorblock.value.valueis undefined. Guard before callingreplace.-const emoji = getCustomEmoji?.(block.value?.value.replace(/\:/g, '')); +const raw = block.value?.value; +const emoji = raw ? getCustomEmoji?.(raw.replace(/:/g, '')) : undefined;
🧹 Nitpick comments (6)
app/containers/markdown/components/emoji/Emoji.tsx (2)
41-41: Minor: simplify space prefix logicMore explicit and avoids falsy truthiness quirks.
-const spaceLeft = index && index > 0 ? ' ' : ''; +const spaceLeft = (index ?? 0) > 0 ? ' ' : '';
80-90: Avoid redundant fontSize sourcesYou pass both
size={...}and text styles (styles.text/styles.textBig) that likely setfontSize. Prefer one source to prevent surprises.app/components/Emoji/Emoji.tsx (3)
5-5: Layering concern: components → containers dependency
components/Emojiimportscontainers/EmojiPicker/CustomEmoji, creating an upward dependency. Consider movingCustomEmojitoapp/componentsor a shared module to keep layers clean.
8-12: Doc the accepted string formsClarify via a short JSDoc that
emoji(string) may be Unicode,:shortname:or bareshortname, and that ASCII emoticons are rendered verbatim. Helps future call-sites.
1-30: Add focused tests for the centralized componentPlease add unit tests covering:
- '😄' (Unicode) → renders unchanged
 - ':smile:' and 'smile' → render 😄
 - ':)' (ASCII) → rendered verbatim
 - Custom emoji object → uses CustomEmoji with size applied
 I can scaffold Jest tests for these cases if helpful.
app/containers/EmojiPicker/Emoji.tsx (1)
3-11: Optional: memoize to reduce list re-rendersEmoji picker lists can be large; wrapping with
React.memo(ormemowith a shallow props check) can help.-import React from 'react'; +import React, { memo } from 'react'; ... -export const EmojiPickerEmoji = ({ emoji }: IEmojiProps): React.ReactElement => ( +export const EmojiPickerEmoji = memo(({ emoji }: IEmojiProps): React.ReactElement => ( <Emoji emoji={emoji} style={typeof emoji === 'string' ? styles.categoryEmoji : styles.customCategoryEmoji} /> -); +));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/Emoji/Emoji.tsx(1 hunks)app/containers/EmojiPicker/Emoji.tsx(1 hunks)app/containers/markdown/components/emoji/Emoji.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/containers/EmojiPicker/Emoji.tsx (1)
app/containers/EmojiPicker/interfaces.ts (1)
IEmojiProps(44-46)
app/components/Emoji/Emoji.tsx (1)
app/definitions/IEmoji.ts (1)
ICustomEmoji(12-15)
🔇 Additional comments (1)
app/containers/EmojiPicker/Emoji.tsx (1)
7-12: LGTM: wrapper cleanly delegates to the shared Emoji componentKeeps styling concerns local and centralizes rendering. Nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/components/Emoji/Emoji.tsx (5)
5-5: Avoid component → container dependency; relocate CustomEmoji to a shared layerImporting from a container path in a shared component can create layering cycles. Consider moving
CustomEmojiunderapp/components/Emoji/CustomEmoji(or a shared module) and updating imports. Keep a re-export at the old path for compatibility if needed.
10-11: Clarify styling API to prevent misuse and casts (add textStyle/imageStyle)Today a single
styleprop is cast to bothTextStyleandImageStyle, which is easy to misuse. Keepstylefor BC, but add explicittextStyle/imageStyleprops and apply them in the respective branches.interface IEmojiProps { emoji: string | ICustomEmoji; - style?: StyleProp<TextStyle | ImageStyle>; + style?: StyleProp<TextStyle | ImageStyle>; // backward compatible + textStyle?: StyleProp<TextStyle>; + imageStyle?: StyleProp<ImageStyle>; size?: number; } -const Emoji = ({ emoji, style, size = 16 }: IEmojiProps) => { +const Emoji = ({ emoji, style, textStyle, imageStyle, size = 16 }: IEmojiProps) => { const { formatShortnameToUnicode } = useShortnameToUnicode(true); @@ - <Text style={[{ fontSize: size }, style as StyleProp<TextStyle>]}> + <Text style={[{ fontSize: size }, style as StyleProp<TextStyle>, textStyle]}> {`${leading}${converted}${trailing}`} </Text> @@ <CustomEmoji - style={[{ width: size, height: size }, style as StyleProp<ImageStyle>]} + style={[{ width: size, height: size }, style as StyleProp<ImageStyle>, imageStyle]} emoji={emoji} />Also applies to: 37-37, 45-45
1-1: Wrap with React.memo to reduce unnecessary re-renders in emoji-heavy listsThis component is frequently used in FlatLists. Memoization gives a cheap win.
-import React from 'react'; +import React, { memo } from 'react'; @@ -export default Emoji; +export default memo(Emoji);Also applies to: 51-51
37-37: Consider disabling font scaling for predictable emoji sizingIf you want consistent emoji sizes independent of system font scaling, set
allowFontScaling={false}(or expose it as a prop defaulting to false).- <Text style={[{ fontSize: size }, style as StyleProp<TextStyle>]}> + <Text allowFontScaling={false} style={[{ fontSize: size }, style as StyleProp<TextStyle>]}>
17-33: Add minimal tests for the conversion matrixRecommend unit tests covering: Unicode (“😄”), colon shortname (:smile:), bare shortname (smile), ASCII (":)"), unknown shortname (":notfound:"), and custom emoji object. I can draft Jest + @testing-library/react-native tests if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/Emoji/Emoji.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/components/Emoji/Emoji.tsx (2)
app/definitions/IEmoji.ts (1)
ICustomEmoji(12-15)app/lib/database/model/CustomEmoji.js (1)
CustomEmoji(8-18)
🔇 Additional comments (1)
app/components/Emoji/Emoji.tsx (1)
17-33: Shortname handling looks correct and resolves the earlier colon-wrapping issue — LGTMColon-delimited and bare shortnames are converted; Unicode and ASCII are preserved; leading/trailing whitespace retained. This addresses the prior review concern.
Also applies to: 36-40
| 
           ✅ All tests have passed locally and the lint-testunit job has completed successfully on CircleCI.  | 
    
| 
           @abhisek7154 we'll get here eventually. Just finishing internal priorities. Thanks for your contribution!  | 
    
… (#6535)
Proposed changes
Refactored emoji rendering by introducing a shared component at
app/components/Emoji/Emoji.tsx.app/containers/EmojiPicker/Emoji.tsxupdated to use the shared component.app/containers/markdown/components/emoji/Emoji.tsxupdated to use the shared component.This removes duplicate logic and ensures consistent rendering of Unicode, shortcode, and custom emojis.
Issue(s)
Closes #6535
How to test or reproduce
:smile::)Screenshots
Types of changes
Checklist
Further comments
This refactor consolidates emoji handling into a single reusable component.
Markdown and EmojiPicker previously duplicated similar logic, which made updates harder and risked inconsistent behavior.
Centralizing this logic ensures:
No new functionality is added — the focus is purely on improving structure and maintainability.
Summary by CodeRabbit